Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix package name handling to retain version and strip ‘@’ suffix #1472

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

eminaktas
Copy link
Contributor

Fix #1471

Dear @jonjohnsonjr,

Can you please review my change? I noticed your PR here. I was also looking in the issue and this change worked.

Signed-off-by: Emin Aktas <eminaktas34@gmail.com>
@jonjohnsonjr
Copy link
Contributor

jonjohnsonjr commented Jan 15, 2025

Closed and re-opened to try to retrigger CI checks, didn't seem to work 🤔

Oh I need to individually "aprove and run" each check now, weird.

@jonjohnsonjr jonjohnsonjr merged commit a4329b9 into chainguard-dev:main Jan 15, 2025
15 checks passed
@eminaktas eminaktas deleted the fix/1471 branch January 16, 2025 18:05
@@ -226,7 +243,11 @@ func unify(originals []string, inputs []resolved) (map[string][]string, map[stri
// package constraint including the operator.
for _, pkg := range sets.List(missing) {
if ver := originalPackages.versions[pkg]; ver != "" {
pl = append(pl, fmt.Sprintf("%s%s", pkg, ver))
if pin := originalPackages.versions[pkg]; pin != "" {
pl = append(pl, fmt.Sprintf("%s%s%s", pkg, ver, pin))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This produces an invalid world when using tagged repositories like @local or @edge.

The order needs to be

- pl = append(pl, fmt.Sprintf("%s%s%s", pkg, ver, pin))
+ pl = append(pl, fmt.Sprintf("%s%s%s", pkg, pin, ver))

as per apk-world(5):

This is a plaintext file with one constraint using dependency notation per line. Each line has the format: name{@tag}{[<>~=]version}.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @emilylange

When I tested it with @local package, the format you shared didn't work. This is the reason I kept that like that. However, changing the order doesn't break the local package build since it is not the case for local built packages.

With @local package repository below example config does not work.

contents:
  repositories:
    - '@local ./packages'
  keyring:
    - melange.rsa.pub
  packages:
    - world@local=7.0.0-r0

This is the error message. This error happens before the changes.

2025/01/24 19:25:15 DEBU got 1 indexes:
./packages/x86_64/APKINDEX.tar.gz
Error: locking config: resolving apk packages: for arch "amd64": solving "world@local=7.0.0-r0" constraint: nothing provides "world@local=7.0.0-r0"
2025/01/24 19:25:15 INFO error during command execution: locking config: resolving apk packages: for arch "amd64": solving "world@local=7.0.0-r0" constraint: nothing provides "world@local=7.0.0-r0"

However, this config works but it doesn't match with the format.

contents:
  repositories:
    - '@local ./packages'
  keyring:
    - melange.rsa.pub
  packages:
    - world=7.0.0-r0@local

Can you share the failing config?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was referring to the resulting /etc/apk/world.

But you are right that this particular line I commented on isn't the culprit, as after looking a lot more into this, I can confidently say that this goes way deeper.

It is inherently confusing to have slight ordering differences across the codebase.

A minimal example and reproducer is:

contents:
  repositories:
    - https://dl-cdn.alpinelinux.org/alpine/edge/main
    - '@testing https://dl-cdn.alpinelinux.org/alpine/edge/testing'
  packages:
    - alpine-base@testing

entrypoint:
  command: /bin/ash -l

archs:
  - amd64

Thanks to this PR here, apko lock and apko build are no longer failing. That's lovely and all.
But it doesn't fix the underlying cause, unfortunately.

The resulting /etc/apk/world look something like this (v0.23.0, based on the APKINDEX at the time of writing):

aef8b91a311a:/# cat /etc/apk/world 
alpine-base=3.22.0_alpha20250108-r0@testing
alpine-baselayout-data=3.6.8-r1
alpine-baselayout=3.6.8-r1
alpine-conf=3.19.2-r0
alpine-keys=2.5-r0
alpine-release=3.22.0_alpha20250108-r0
apk-tools=2.14.9-r0
busybox-binsh=1.37.0-r13
busybox-ifupdown=1.37.0-r13
busybox-mdev-openrc=1.37.0-r13
busybox-openrc=1.37.0-r13
busybox-suid=1.37.0-r13
busybox=1.37.0-r13
ca-certificates-bundle=20241121-r1
libcap2=2.73-r0
libcrypto3=3.3.2-r4
libssl3=3.3.2-r4
mdev-conf=4.7-r0
musl-utils=1.2.5-r9
musl=1.2.5-r9
openrc=0.56-r0
scanelf=1.3.8-r1
ssl_client=1.37.0-r13
zlib=1.3.1-r2

As pointed out in my previous comment, alpine-base=3.22.0_alpha20250108-r0@testing is an invalid entry as per apk-world(5).

If you run apk upgrade, apk fix or apk add <some other package> in a given container with that /etc/apk/world, apk will remove that entry and dependencies.

aef8b91a311a:/# apk upgrade
fetch https://dl-cdn.alpinelinux.org/alpine/edge/testing/x86_64/APKINDEX.tar.gz
fetch https://dl-cdn.alpinelinux.org/alpine/edge/main/x86_64/APKINDEX.tar.gz
(1/24) Purging alpine-base (3.22.0_alpha20250108-r0)
(2/24) Purging alpine-baselayout (3.6.8-r1)
(3/24) Purging alpine-baselayout-data (3.6.8-r1)
(4/24) Purging alpine-release (3.22.0_alpha20250108-r0)
(5/24) Purging alpine-keys (2.5-r0)
(6/24) Purging apk-tools (2.14.9-r0)
(7/24) Purging ca-certificates-bundle (20241121-r1)
(8/24) Purging busybox-mdev-openrc (1.37.0-r13)
(9/24) Purging mdev-conf (4.7-r0)
(10/24) Purging busybox-openrc (1.37.0-r13)
(11/24) Purging busybox-suid (1.37.0-r13)
(12/24) Purging musl-utils (1.2.5-r9)
(13/24) Purging scanelf (1.3.8-r1)
(14/24) Purging alpine-conf (3.19.2-r0)
(15/24) Purging openrc (0.56-r0)
(16/24) Purging busybox-binsh (1.37.0-r13)
(17/24) Purging busybox (1.37.0-r13)
(18/24) Purging libcap2 (2.73-r0)
(19/24) Purging busybox-ifupdown (1.37.0-r13)
(20/24) Purging zlib (1.3.1-r2)
(21/24) Purging ssl_client (1.37.0-r13)
(22/24) Purging libssl3 (3.3.2-r4)
(23/24) Purging libcrypto3 (3.3.2-r4)
(24/24) Purging musl (1.2.5-r9)
OK: 17592186044408 MiB in 0 packages
aef8b91a311a:/# cat /etc/apk/world 
/bin/ash: cat: not found
aef8b91a311a:/# read file < /etc/apk/world ; echo $file

aef8b91a311a:/#

With alpine-base being the only package in the example provided, apk will delete everything, resulting in an unusable environment.

So far, I came up with the following diff to fix the underlying cause.
Though I would have to check

diff --git a/pkg/apk/apk/version.go b/pkg/apk/apk/version.go
index 185dbf7..5d88922 100644
--- a/pkg/apk/apk/version.go
+++ b/pkg/apk/apk/version.go
@@ -37,7 +37,7 @@ import (
 
 var (
        versionRegex     = regexp.MustCompile(`^([0-9]+)((\.[0-9]+)*)([a-z]?)((_alpha|_beta|_pre|_rc)([0-9]*))?((_cvs|_svn|_git|_hg|_p)([0-9]*))?((-r)([0-9]+))?$`)
-       packageNameRegex = regexp.MustCompile(`^([^@=><~]+)(([=><~]+)([^@]+))?(@([a-zA-Z0-9]+))?$`)
+       packageNameRegex = regexp.MustCompile(`^([^@=><~]+)(@([a-zA-Z0-9]+))?(([=><~]+)([^@]+))?$`)
 )
 
 func init() {
@@ -367,12 +367,12 @@ func resolvePackageNameVersionPin(pkgName string) parsedConstraint {
        // layout: [full match, name, =version, =|>|<, version, @pin, pin]
        p := parsedConstraint{
                name:    parts[0][1],
-               version: parts[0][4],
-               pin:     parts[0][6],
+               pin:     parts[0][3],
+               version: parts[0][6],
                dep:     versionAny,
        }
 
-       matcher := parts[0][3]
+       matcher := parts[0][5]
        if matcher != "" {
                // we have an equal
                switch matcher {
diff --git a/pkg/apk/apk/version_test.go b/pkg/apk/apk/version_test.go
index 1e99665..9e6afd4 100644
--- a/pkg/apk/apk/version_test.go
+++ b/pkg/apk/apk/version_test.go
@@ -933,8 +933,8 @@ func TestResolverPackageNameVersionPin(t *testing.T) {
                {"name<1.2.3", "name", "1.2.3", versionLess, ""},
                {"name>=1.2.3", "name", "1.2.3", versionGreaterEqual, ""},
                {"name<=1.2.3", "name", "1.2.3", versionLessEqual, ""},
-               {"name@edge=1.2.3", "name@edge=1.2.3", "", versionAny, ""}, // wrong order, so just returns the whole thing
-               {"name=1.2.3@community", "name", "1.2.3", versionEqual, "community"},
+               {"name@edge=1.2.3", "name", "1.2.3", versionEqual, "edge"},
+               {"name=1.2.3@community", "name=1.2.3@community", "", versionAny, ""}, // wrong order, so just returns the whole thing
        }
 
        for _, tt := range tests {
diff --git a/pkg/build/lock.go b/pkg/build/lock.go
index e00e858..c36e096 100644
--- a/pkg/build/lock.go
+++ b/pkg/build/lock.go
@@ -145,8 +145,8 @@ func unify(originals []string, inputs []resolved) (map[string][]string, map[stri
                }
 
                // Extract pinned version if present
-               if idx := strings.IndexAny(orig, "@"); idx >= 0 {
-                       pinned = orig[idx:]
+               if idx := strings.IndexAny(name, "@"); idx >= 0 {
+                       pinned = name[idx:]
                }
 
                // Remove pinned suffix from name and version
@@ -256,10 +256,11 @@ func unify(originals []string, inputs []resolved) (map[string][]string, map[stri
        // Append all of the resolved and unified packages with an exact match
        // based on the resolved version we found.
        for _, pkg := range sets.List(acc.packages) {
-               pkgName := fmt.Sprintf("%s=%s", pkg, acc.versions[pkg])
+               pkgName := pkg
                if pin := originalPackages.pinned[pkg]; pin != "" {
                        pkgName = fmt.Sprintf("%s%s", pkgName, pin)
                }
+               pkgName = fmt.Sprintf("%s=%s", pkgName, acc.versions[pkg])
                pl = append(pl, pkgName)
        }
        // Sort the package list explicitly with the `=` included.
@@ -274,10 +275,11 @@ func unify(originals []string, inputs []resolved) (map[string][]string, map[stri
        for _, input := range inputs {
                pl := make([]string, 0, len(input.packages))
                for _, pkg := range sets.List(input.packages) {
-                       pkgName := fmt.Sprintf("%s=%s", pkg, input.versions[pkg])
+                       pkgName := pkg
                        if pin := originalPackages.pinned[pkg]; pin != "" {
                                pkgName = fmt.Sprintf("%s%s", pkgName, pin)
                        }
+                       pkgName = fmt.Sprintf("%s=%s", pkgName, input.versions[pkg])
                        pl = append(pl, pkgName)
                }
                // Sort the package list explicitly with the `=` included.
@@ -304,4 +306,4 @@ func unify(originals []string, inputs []resolved) (map[string][]string, map[stri
 }
 
 // Copied from go-apk's version.go
-var packageNameRegex = regexp.MustCompile(`^([^@=><~]+)(([=><~]+)([^@]+))?(@([a-zA-Z0-9]+))?$`)
+var packageNameRegex = regexp.MustCompile(`^([^@=><~]+)(@([a-zA-Z0-9]+))?(([=><~]+)([^@]+))?$`)

Though given the lack of code coverage, this could also cause regressions in some unexpected way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emilylange Thanks for the detailed explanation! I think this needs its own ticket and maybe even a discussion in the #apko Slack channel. Since this is a merged ticket, there’s a good chance the right people might not see it. Would be great to get more visibility on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unable to lock packages to a consistent version: map[myapp@local:[ (amd64)]]
3 participants